-
Notifications
You must be signed in to change notification settings - Fork 428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(webhooks): [nan 1064] webhook on sync error #2281
Conversation
@@ -0,0 +1,153 @@ | |||
/* eslint-disable @typescript-eslint/unbound-method */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copied over from shared
|
||
dayjs.extend(utc); | ||
|
||
export const sendSync = async ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Logic essentially copied over from shared
@@ -103,6 +106,7 @@ export default class SyncRun { | |||
recordsService: RecordsServiceInterface; | |||
dryRunService?: NangoProps['dryRunService']; | |||
slackNotificationService?: SlackService; | |||
sendSyncWebhook?: (params: SendSyncParams) => Promise<void>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I am gonna propose to Bastien a refactor run.service
project because it has become a mammoth class that does everything. Its run
function is 500+ lines I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it is a beast. Definitely needed.
@@ -4,12 +4,10 @@ import dayjs from 'dayjs'; | |||
import utc from 'dayjs/plugin/utc.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what still live in this file? Is everything moved over to the webhooks package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
webhook forwarding logic which I'll bring over in another PR.
export interface ErrorPayload { | ||
type: string; | ||
description: string; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this an api related type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
internal API I would say
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we can keep this file for http api, maybe rename the file if clearer 👌🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🦾 Minor comments
export interface ErrorPayload { | ||
type: string; | ||
description: string; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we can keep this file for http api, maybe rename the file if clearer 👌🏻
@@ -420,11 +461,10 @@ export default class SyncRun { | |||
} | |||
|
|||
const startTime = Date.now(); | |||
const syncStartDate = new Date(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be moved at the very beginning of the method or could be a property of the class directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will make a note to follow up on this later
Describe your changes
Add in logic to send webhooks on sync failure
Issue ticket number and link
NAN-1064
Checklist before requesting a review (skip if just adding/editing APIs & templates)